Skip to content

Update transport model creation for new class structure#223

Merged
GDYendell merged 1 commit intomainfrom
tranport-model
Oct 1, 2025
Merged

Update transport model creation for new class structure#223
GDYendell merged 1 commit intomainfrom
tranport-model

Conversation

@GDYendell
Copy link
Contributor

@GDYendell GDYendell commented Sep 30, 2025

No description provided.

@GDYendell GDYendell mentioned this pull request Oct 1, 2025
Merged
1 task
hyperrealist
hyperrealist approved these changes Oct 1, 2025
View reviewed changes
Copy link
Contributor

@hyperrealist hyperrealist left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think everything looks good, but just one thought: should we rename the base Transport class to BaseTransport or something like that?

FastCS/src/fastcs/transport/transport.py

Line 10 in e944113

class Transport:

Because it is placed earlier in the hierarchy than the actual transports it could be a bit confusing to new users.

from fastcs.transport import BaseTransport
# vs
from fastcs.transport.epics.ca.transport import EpicsCATransport

This will stylistically align with other parts of the codebase like BaseController too. Perhaps needs more consideration.

@GDYendell
Copy link
Contributor Author

GDYendell commented Oct 1, 2025

should we rename the base Transport class to BaseTransport

Yeah it is something to consider. Though the reason BaseController exists currently is because Controller and SubController inherit it, so it is not quite the same. But, it might be a bit nicer if we have BaseTransport and then useTransport as the union of all transports for the model generation. I will leave it as is for now.

@GDYendell GDYendell merged commit cc11492 into main Oct 1, 2025
6 of 10 checks passed
@GDYendell GDYendell deleted the tranport-model branch October 1, 2025 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hyperrealist hyperrealist hyperrealist approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@GDYendell @hyperrealist